Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement per-parentchain extrinsic factory and metadata #1501

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

brenzi
Copy link
Collaborator

@brenzi brenzi commented Nov 20, 2023

needed to support Asset Hub (or Encointer) as Target A parentchain

major steps:

  • per-parentchain Extrinsic Type (especially AssetTip)
  • per-parentchian metadata (pallet indices)

@brenzi brenzi changed the title implemented per-parentchain extrinsic factory implement per-parentchain extrinsic factory and metadata Nov 20, 2023
Comment on lines +69 to +86
where
ParentchainApi: AccountApi<AccountId = AccountId, Balance = Balance, Index = Index>
+ GetBalance<Balance = Balance>
+ GetTransactionPayment<Balance = Balance>
+ BalancesExtrinsics<
Address = Address,
Balance = Balance,
Extrinsic<TransferAllowDeathCall<Address, Balance>> = UncheckedExtrinsicV4<
Address,
TransferAllowDeathCall<Address, Balance>,
<ParentchainApi as ChainApi>::Signature,
<ParentchainApi as ChainApi>::SignedExtra,
>,
> + SubmitAndWatch<Hash = Hash>
+ ChainApi<Signer = sr25519::Pair>,
ParentchainApi::Signature: Encode,
ParentchainApi::SignedExtra: Encode,
{
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is so horribly verbose. there has to be another way

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can create an aggregate trait like here: https://stackoverflow.com/a/26983395/19331449

(parentchain_handler, last_synced_header)
}

fn init_integritee_parentchain<E>(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was just an attempt to solve the mess with redundant but explict code. not sure that's the way

}
}

fn send_extrinsic(
fn send_integritee_extrinsic(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to be generic over parentchains. we only need this for Integritee Network

pub type TargetBRuntimeConfig =
WithExtrinsicParams<AssetRuntimeConfig, target_b::ParentchainExtrinsicParams>;

pub enum ParentchainApiLocal {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this I somehow like and it seems to work for OCall. Maybe we could make it work for service-main instead of the ParentchainApiWrapper shizzle?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think this is the way to go actually for many things, also when we distinguish build flavors.

Comment on lines +102 to +103
// fixme: this is an ugly hack because api-client's 'new()' isn't part of any trait we could implement therefore we can't use the fn new() on generic api types
impl ParentchainApiWrapper for IntegriteeParentchainApiWrapper {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really ugly. there has to be a better way. but we may need to patch api-client

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapper not necessary, we can just define our own trait and implement it on the api client IMO, which is a bit better.

@@ -103,7 +106,7 @@ impl TargetAParachainHandler {
extrinsics_factory.clone(),
)?,
WorkerMode::Sidechain =>
unimplemented!("Can't run target a chain in sidechain mode yet."),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything I fail to see why this will not work?

@brenzi
Copy link
Collaborator Author

brenzi commented Nov 22, 2023

@clangenb I think I need an intermediate review as I seem to get something entirely wrong.

status:

  • a-priori: master starts against Zombienet with Assethub. Everything on Integritee works fine and sidechain blocks are produced, shard vault created a.s.o., but extrinsic to Asset Hub to fund the enclave fails, as has to be expected. So that's a healthy starting point IMO
  • this WIP still has introduced a lot of redundant code, a cleanup will be necessary once it works.
  • enclave-runtime builds in SW,sidechain,dcap mode with per-parentchain extrinsic factories and ocall refactoring seems fine
  • service main is a PITA. I'm particularly concerned about one of your remarks here:
    // #TODO: #1451: Reintroduce `ParentchainApi: ChainApi` once there is no trait bound conflict
    // any more with the api-clients own trait definitions.

reproduce my errors in root folder build: SGX_MODE=SW WORKER_MODE=sidechain WORKER_FEATURES=dcap make

@@ -40,8 +40,10 @@ log = { version = "0.4", default-features = false }
# substrate dep
binary-merkle-tree = { default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v0.9.42" }
frame-support = { default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v0.9.42" }
frame-system = { default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v0.9.42" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std enablement

@@ -20,6 +20,7 @@
#[cfg(all(feature = "std", feature = "sgx"))]
compile_error!("feature \"std\" and feature \"sgx\" cannot be enabled at the same time");

extern crate core;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

Comment on lines +102 to +103
// fixme: this is an ugly hack because api-client's 'new()' isn't part of any trait we could implement therefore we can't use the fn new() on generic api types
impl ParentchainApiWrapper for IntegriteeParentchainApiWrapper {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapper not necessary, we can just define our own trait and implement it on the api client IMO, which is a bit better.

pub type TargetBRuntimeConfig =
WithExtrinsicParams<AssetRuntimeConfig, target_b::ParentchainExtrinsicParams>;

pub enum ParentchainApiLocal {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think this is the way to go actually for many things, also when we distinguish build flavors.

Comment on lines 68 to +70
// #TODO: #1451: Reintroduce `ParentchainApi: ChainApi` once there is no trait bound conflict
// any more with the api-clients own trait definitions.
impl<EnclaveApi> ParentchainHandler<ParentchainApi, EnclaveApi>
impl<EnclaveApi> ParentchainHandler<IntegriteeParentchainApi, EnclaveApi>
Copy link
Contributor

@clangenb clangenb Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are exactly right. I believe this is the root cause of the problem. The api-client has re-defined types and traits because they were not available in no-std in substrate < v0.9.43. When I updated the api-client to its newest version, but v0.9.42, I had to reintroduce these types again: scs/substrate-api-client@ea49a0d.

Then I was essentially in the same rabbit whole as you: certain trait-bounds weren't satisfied because we are using the actual types from substrate whereas the api-client uses its types/traits in its traitbounds.

My recommedation is: if you have to use generics in those api taits, we unfortunately have to upgrade the polkadot version and the api-client.

@brenzi
Copy link
Collaborator Author

brenzi commented Nov 23, 2023

will pause work here, hoping for teaclave to bump rustc so we can upgrade to Polkadot 1.3.0 and api-client 0.15.0
trait bound trouble should be easier to fix nicely then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants